-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Filter.Reduce (general dimension reduction for ImageStack) #1342
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
==========================================
+ Coverage 89.21% 89.25% +0.04%
==========================================
Files 148 149 +1
Lines 5377 5416 +39
==========================================
+ Hits 4797 4834 +37
- Misses 580 582 +2
Continue to review full report at Codecov.
|
---------- | ||
dims : Axes | ||
one or more Axes to project over | ||
func : Union[str, Callable] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not accept just Callable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like it. this is the more 'pythonic' way in scientific computing -- will be easier for numpy/pandas users to reason about i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dganguli You like what? The original or my suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the absence of an opposition, I think func
should be a Callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's nice to have the string option. I think a lot of users will find it annoying to need to import numpy.amax() and this component to perform a maximum intensity projection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the string option, just for ecosystem continuity -- users are used to passing function names that are resolved by getattr, as this formalism is used a lot in scipy.spatial.distance
among other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome. is the plan to re-write max_proj to call reduce with 'max' or 'np.max'? or are we ditching max_proj?
@dganguli , I think @ttung suggested that we keep |
@ttung , I think I've addressed your comments. However, the test is failing during doc generation (see error below) and I'm not sure what it is expecting because it passes
|
@ttung, I think I have addressed your comments. Would you mind taking another look? |
---------- | ||
dims : Axes | ||
one or more Axes to project over | ||
func : Union[str, Callable] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the absence of an opposition, I think func
should be a Callable.
Summary
As discussed in #1302, this component provides the ability to perform dimension reduction operations to
ImageStack
usingDataArray.reduce()
. Currently, max, mean, sum, and a user-provided can be applied across specified dimensions. Closes #1302.Testing
Testing of sum, mean, and max are performed in
starfish/core/image/_filter/test/test_reduce.py
To Do